Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reformat global fee #2229

Closed
wants to merge 9 commits into from
Closed

Reformat global fee #2229

wants to merge 9 commits into from

Conversation

sainoe
Copy link
Contributor

@sainoe sainoe commented Feb 21, 2023

Update the code to be more aligned with Go conventions.
Some functions were simplified and renamed but their logic stayed untouched.


Note that the required amount of `uatom` in globalfee is overwritten by the amount in minimum-gas-prices.
Also, the `1stake` in minimum-gas-prices is ignored.
Note that the required amounts of `uatom` and `stake` in minimum-gas-prices is are ignored.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yaruwangway did I get it right?

Copy link
Contributor

@yaruwangway yaruwangway Feb 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"1stake" is ignore due to the denom is not in globalfee fee
"uatom" is not ignored, but rather when combining global fee, it is being "merged", so globalfee provides 0.1uatom, while minimum-gas-prices wants 0.2 uatom, so 0.2 uatom is taken to check the paid fees. since this testcase is for bypass msg, only the denom "uatom" is taken to check if the paid fee is zero, if not zero, if the paid fee denom is in "uatom'

@codecov
Copy link

codecov bot commented Feb 21, 2023

Codecov Report

Merging #2229 (d9f5e41) into main (297cdb9) will increase coverage by 0.87%.
The diff coverage is 84.21%.

❗ Current head d9f5e41 differs from pull request most recent head 8477af7. Consider uploading reports for the commit 8477af7 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2229      +/-   ##
==========================================
+ Coverage   84.76%   85.63%   +0.87%     
==========================================
  Files          21       21              
  Lines        1483     1455      -28     
==========================================
- Hits         1257     1246      -11     
+ Misses        181      167      -14     
+ Partials       45       42       -3     
Impacted Files Coverage Δ
x/globalfee/module.go 83.92% <0.00%> (-4.76%) ⬇️
x/globalfee/ante/fee.go 85.14% <84.84%> (-2.86%) ⬇️
x/globalfee/ante/fee_utils.go 97.97% <100.00%> (+9.18%) ⬆️
x/globalfee/types/params.go 100.00% <100.00%> (+21.05%) ⬆️

// containsOnlyBypassMinFeeMsgs returns true if all the given msgs type are listed
// in the BypassMinFeeMsgTypes of the FeeDecorator.
func (mfd FeeDecorator) containsOnlyBypassMinFeeMsgs(msgs []sdk.Msg) bool {
for _, msg := range msgs {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @sainoe , good point, can you revert back here and move containsOnlyBypassMinFeeMsgs to this pr #2218 for bypass-msg refactor

func (coins DecCoins) Validate() error {
switch len(coins) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this Validate will later refactor to check only positive amount, zero coins are not allowed.

err := types.RegisterQueryHandlerClient(context.Background(), mux, types.NewQueryClient(clientCtx))
if err != nil {
// same behavior as in cosmos-sdk
panic(err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

@yaruwangway
Copy link
Contributor

Hi, changed the base branch and moved to here #2242

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants